editoast: fix a couple issues in new authentication middlewares#16980
editoast: fix a couple issues in new authentication middlewares#16980leovalais wants to merge 2 commits into
Conversation
The middleware pushed in request extensions the user and the roles of the impersonator instead of the impersonated user. - Reworks and simplifies the flow of the middleware. - Fixes impersonation processing. - Adds a test for `/authz/me` to have a bit of impersonation coverage. - Adds a request extension `.impersonate`. - Removes useless referencing of `impl AsRef`. Signed-off-by: Léo VALAIS <leovalais+git@proton.me>
- Makes sure the user is created if it doesn't exist. - More consistent behavior when compared to other authentication modes. - Adds a test for `/authz/me` to inspect the behavior. Signed-off-by: Léo VALAIS <leovalais+git@proton.me>
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 1)] | ||
| async fn whoami_impersonation() { |
There was a problem hiding this comment.
nit: maybe also test that non-admin users cannot impersonate
| conn.transaction(async |conn| { | ||
| let user = | ||
| editoast_models::User::retrieve_by_identity(identity, conn.clone()).await?; | ||
| warn_mismatched_name(&user, identity, name); |
There was a problem hiding this comment.
nit: the warn_mismatched_name might be a bit confusing here. I would interpret it as "always send a warning about the user name at this point of execution" when it actually checks whether there is a name mismatch and only logs a warning if there is one.
Maybe rename it to check_name_mismatch, warn_on_name_mismatch or something alike ?
| if let Some(impersonator) = impersonator { | ||
| check_impersonation_privilege(regulator.openfga(), &impersonator).await?; | ||
| } else { | ||
| register_origin_user(conn.clone(), (impersonator_identity, impersonator_name)) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
If the impersonator does not exist yet we skip the impersonation check and create the impersonator instead ? That feels wrong
If I'm not mistaken it means that trying to impersonate with a user which does not exist always succeeds even though it is supposed to be reserved to admins
| @@ -149,54 +150,83 @@ pub(in crate::views) async fn authentication_validation_middleware( | |||
| Ok((Some(user), ::authz::v2::subject_roles(authz_user))) | |||
There was a problem hiding this comment.
If we only call that method on nonexistent user they should not have roles yet aswell ? Maybe we can either directly return an empty roles list keep retrieving the roles but in order to check that the retrieved roles list is indeed empty and panic if it is not ?
2 participants
refs https://github.com/osrd-project/osrd-confidential/issues/1168
To merge after #16281